Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reference Specification for API first approach #93

Merged
merged 17 commits into from
Nov 25, 2021
Merged

Conversation

wtrocki
Copy link
Member

@wtrocki wtrocki commented Oct 13, 2021

Landing as first version/draft for discussion.
Some styles/links will be improved further.
I have also draft blog post to accompany this spec.

wtrocki and others added 2 commits October 13, 2021 17:15
Co-authored-by: IgorTodorovskiIBM <39890068+IgorTodorovskiIBM@users.noreply.github.com>
Co-authored-by: IgorTodorovskiIBM <39890068+IgorTodorovskiIBM@users.noreply.github.com>
docs/development/apifirst.md Outdated Show resolved Hide resolved
docs/development/apifirst.md Outdated Show resolved Hide resolved
@mhdawson
Copy link
Contributor

mhdawson commented Nov 1, 2021

@wtrocki I pushed a commit with a substantial edit (I could not easily figure out how to do a PR against your PR).

We can discard if the edit does not make sense to you.

docs/development/apifirst.md Outdated Show resolved Hide resolved
docs/development/apifirst.md Outdated Show resolved Hide resolved
@wtrocki
Copy link
Member Author

wtrocki commented Nov 2, 2021

Provided changes looks good, but I'm not 100% sure if we want to rebrand this as API guide. Maybe we can find sweet spot between "building your API" vs "API first".

My only concern is that many developers can look into this chapter as "Building your API with express" and it will find very opiniated and unfamiliar concept for them to look at.

@mhdawson
Copy link
Contributor

mhdawson commented Nov 2, 2021

@wtrocki maybe we can add some additional words. The sections we have are "the team has had success with X". If our main success is with API first using express then it makes sense that this is what is in this section. Instead of narrowing the title I think it would be better to expand the content if there are other approaches/tools that we have used regularly and have had success with. I do think we want a topic with our guideance/experience with how to build a REST API.

@mhdawson
Copy link
Contributor

mhdawson commented Nov 2, 2021

The other thing I should have mentioned is that we should add the referenced modules to the npcheck.json file.

Co-authored-by: Wojciech Trocki <wtrocki@redhat.com>
@wtrocki
Copy link
Member Author

wtrocki commented Nov 10, 2021

npcheck.json file.

npmcheck.json seems to be non functional right now and failing on build. Can I get help with adding those packages and fixing build? See #94

@mhdawson
Copy link
Contributor

Getting npcheck working again on the repo is a priority. There is some ongoing discussion here: https://github.com/nodeshift/npcheck/pu](https://github.com/nodeshift/npcheck/pull/88. @richardlau has been out the last week though and I've had lots of other things so have had time to look. We should try to make sure we resolve in some way next week.

@wtrocki
Copy link
Member Author

wtrocki commented Nov 23, 2021

Done npmcheck.json Can we get this merged?

Copy link
Member

@helio-frota helio-frota left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some minor typos found 👍

Co-authored-by: Helio Frota <00hf11@gmail.com>
npcheck.json Outdated Show resolved Hide resolved
npcheck.json Outdated Show resolved Hide resolved
docs/development/apifirst.md Outdated Show resolved Hide resolved
wtrocki and others added 2 commits November 23, 2021 12:33
Co-authored-by: Richard Lau <rlau@redhat.com>
Co-authored-by: Helio Frota <00hf11@gmail.com>
@wtrocki
Copy link
Member Author

wtrocki commented Nov 23, 2021

Need help with npmcheck

Copy link
Contributor

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with change of name to REST API Development as agreed in meeting today.

@mhdawson mhdawson dismissed helio-frota’s stale review November 24, 2021 16:18

These were addressed

@helio-frota
Copy link
Member

probably need to add these 2 packages in npcheck.json?

"tslib@2.3.1" ---- package which is under the non-acceptable license "0BSD".
"caniuse-lite@1.0.30001282" ---- package which is under the non-acceptable license "CC-BY-4.0".

@helio-frota helio-frota self-requested a review November 24, 2021 16:55
@wtrocki
Copy link
Member Author

wtrocki commented Nov 24, 2021

@helio-frota can you help me with those. I did not added those packages myself.

@helio-frota
Copy link
Member

helio-frota commented Nov 24, 2021

@wtrocki I think you just need to update npcheck.json with

stoplight/prism-http and openapi-editor same as you did for the others to exclude the licenses 👍

(47): The module "@stoplight/prism-http" depends on the "tslib@2.3.1" package which is under the non-acceptable license "0BSD". - ERROR
(48): The module "@stoplight/prism-http" is not tested by community CITGM runs.
...
(56): The module "openapi-editor" depends on the "caniuse-lite@1.0.30001282" package which is under the non-acceptable license "CC-BY-4.0". - ERROR

like this https://github.com/nodeshift/nodejs-reference-architecture/pull/93/files#diff-b43bb8fc5cc3dd0ad9082a957b458e42e6d93d373577c112484736c3199fbea4R194

@mhdawson
Copy link
Contributor

mhdawson commented Nov 24, 2021

@wtrocki looked at the remaining 2 licence failure reports and the overall addition to the rules. 0BSD seems fine to me as it is less restrictive than the BSD ones we allow by default and the other 2 (Python-2.0 and CC-BY-4.0) we've already allowed exceptions for other components since they don't look like they'd be an issue.

I went ahead and update to allow thosteand the "due dilligence" is now green.

@wtrocki
Copy link
Member Author

wtrocki commented Nov 25, 2021

@mhdawson thank you so much! Let's merge that. I will

  • Update website
  • follow up with blog post to redhat-developers.

@wtrocki wtrocki merged commit baf520d into nodeshift:main Nov 25, 2021
@wtrocki wtrocki deleted the api branch November 25, 2021 10:10
@mhdawson
Copy link
Contributor

Pushed a commit on main branch to rename to "REST API Development" based on what we agreed in ref arch meeting this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants